Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

QDevil QDAC-II driver #110

Merged
merged 16 commits into from
Dec 21, 2021
Merged

QDevil QDAC-II driver #110

merged 16 commits into from
Dec 21, 2021

Conversation

jpsecher
Copy link
Contributor

@jpsecher jpsecher commented Nov 9, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021 •
edited
Loading

Codecov Report

Merging #110 (f891c67) into master (a71565e) will increase coverage by 13.22%.
The diff coverage is 94.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #110       +/-   ##
===========================================
+ Coverage    6.92%   20.14%   +13.22%     
===========================================
  Files          97      119       +22     
  Lines       12264    14455     +2191     
===========================================
+ Hits          849     2912     +2063     
- Misses      11415    11543      +128
Impacted Files Coverage Δ
qcodes_contrib_drivers/tests/QDevil/common.py 0.00% <0.00%> (ø)
...ontrib_drivers/tests/QDevil/real_qdac2_fixtures.py 34.48% <34.48%> (ø)
...b_drivers/tests/QDevil/test_real_qdac2_ieee_std.py 42.85% <42.85%> (ø)
...contrib_drivers/tests/QDevil/sim_qdac2_fixtures.py 80.00% <80.00%> (ø)
qcodes_contrib_drivers/drivers/QDevil/QDAC2.py 96.33% <96.33%> (ø)
...contrib_drivers/tests/QDevil/test_sim_qdac2_awg.py 100.00% <100.00%> (ø)
...ib_drivers/tests/QDevil/test_sim_qdac2_channels.py 100.00% <100.00%> (ø)
...trib_drivers/tests/QDevil/test_sim_qdac2_common.py 100.00% <100.00%> (ø)
...rib_drivers/tests/QDevil/test_sim_qdac2_current.py 100.00% <100.00%> (ø)
...ib_drivers/tests/QDevil/test_sim_qdac2_external.py 100.00% <100.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a71565e...f891c67. Read the comment docs.

@FarBo
Copy link
Contributor

FarBo commented Nov 11, 2021

Hi @jpsecher
Thanks for the PR. Let's try to fix the issues in CI tests (issues seems in building docs). Let me know if you need help fixing them.

After that, we will try to review the PR.

@jpsecher
Copy link
Contributor Author

I don't know how to fix the doc build, so I will remove all documentation for now.

@jenshnielsen
Copy link
Collaborator

@jpsecher That seems like a shame. You can disable the execution of the notebooks as documented here https://qcodes.github.io/Qcodes/examples/writing_drivers/Creating-Instrument-Drivers.html#Documentation I think that should be enough to get it building

@jpsecher
Reinstate docs
fd5aa34 
- Make sphinx ignore the exambles
- Add headings to all examples
- Extend index
@FarBo
Copy link
Contributor

FarBo commented Nov 19, 2021

@jpsecher

I started looking into the QDAC2.py. As the first general comment, It would be nice if you could add docstring min. for public functions/ classes.
Also, since some of the arguments are static-typed and some not, Is it possible to type those type-missing arguments?

@jpsecher
Copy link
Contributor Author

jpsecher commented Nov 25, 2021 •
edited
Loading

@FarBo

Hi, I believe I have made all the improvements that I can, thanks!

astafan8
astafan8 approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpsecher sorry for the delay in review! I had a look and it's ready to merge. Just left a few clarification comments on the todo items in the code.

qcodes_contrib_drivers/drivers/QDevil/QDAC2.py Outdated Show resolved Hide resolved
qcodes_contrib_drivers/drivers/QDevil/QDAC2.py Outdated Show resolved Hide resolved
@astafan8 astafan8 enabled auto-merge December 7, 2021 11:33
@astafan8 astafan8 merged commit 2f97777 into QCoDeS:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astafan8 astafan8 astafan8 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jpsecher @codecov-commenter @FarBo @jenshnielsen @astafan8